-
Notifications
You must be signed in to change notification settings - Fork 2
✨ server: add pax for platinum to signature card upgrades #670
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 2a08bc7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Summary of ChangesHello @aguxez, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the server's card issuance API by integrating with PAX for users who are upgrading from a Platinum card to a Signature card. The system now identifies these upgrade scenarios, retrieves necessary user information from Persona, and then registers the user with PAX for associated benefits, all while ensuring that the card issuance flow remains resilient to external service failures. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
WalkthroughDetects platinum→signature upgrades during signature card issuance; after issuance, retrieves Persona account, derives an internalId, and calls PAX's addCapita to provision travel insurance. Adds tests, updates PAX mocks and util signature, and adds a changeset. (45 words) Changes
Sequence DiagramsequenceDiagram
participant Client
participant CardAPI as Card API
participant DB as Database
participant Persona as Persona Service
participant PAX as PAX Service
participant Sentry as Sentry
Client->>CardAPI: Request signature card issuance
CardAPI->>DB: Query credential cards (ACTIVE, FROZEN, DELETED)
DB-->>CardAPI: Return cards
alt Deleted PLATINUM card detected
CardAPI->>CardAPI: mark isUpgradeFromPlatinum
end
CardAPI->>DB: Create new signature card
DB-->>CardAPI: Card created
alt isUpgradeFromPlatinum
CardAPI->>Persona: getAccount(credentialId)
Persona-->>CardAPI: account & identity docs
CardAPI->>CardAPI: validate identity, derive internalId
CardAPI->>PAX: addCapita(capitaPayload)
alt PAX error
PAX-->>CardAPI: error
CardAPI->>Sentry: captureException(error, context)
else PAX success
PAX-->>CardAPI: success
end
end
CardAPI-->>Client: return card response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
The pull request introduces functionality to track platinum to signature card upgrades by integrating with the PAX system. The changes include adding logic to detect such upgrades, calling an external API (addCapita) with user data, and comprehensive test cases covering various scenarios, including successful integration, no-upgrade cases, and error handling for external API calls. The addition of vi.clearAllMocks() in the test suite's afterEach hook is a good practice for ensuring test isolation and reliability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@server/api/card.ts`:
- Around line 383-385: The call to addCapitaForPlatinumUpgrade(credentialId,
account) is intended as fire-and-forget but currently returns a floating
promise; prefix the call with the void operator to make the intent explicit and
satisfy linters (i.e., change the invocation inside the isUpgradeFromPlatinum
block to void addCapitaForPlatinumUpgrade(credentialId, account)). Ensure you do
not await it and keep any surrounding logic unchanged so card creation remains
non-blocking.
In `@server/test/mocks/pax.ts`:
- Around line 3-4: The imports in server/test/mocks/pax.ts are out of order;
move external-library imports before relative imports so "valibot" (InferInput)
is imported before the local "../../utils/pax" (CapitaRequest), ensuring import
order follows eslint-plugin-import conventions (external libs first, then
relative paths) and updating any surrounding import groups similarly.
|
✅ All tests passed. |
e6c1e66 to
1be9a29
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
8f1e6a5 to
1779bbd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@server/api/card.ts`:
- Around line 334-355: The lookup for deletedPlatinumCard is using the
module-level database client instead of the request context DB; replace the call
to database.query.cards.findFirst with c.var.db.query.cards.findFirst (keeping
the same where/filter using cards, credentialId and PLATINUM_PRODUCT_ID) so the
route uses the injected DB client; also update the other similar lookup(s)
around the later block (the occurrences mentioned at 369-372) to use
c.var.db.query.* instead of database.query.* (ensure you keep the same
columns/where predicates and variable names like deletedPlatinumCard and
isUpgradeFromPlatinum).
1779bbd to
cc3ea01
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@server/test/api/card.test.ts`:
- Around line 83-86: The afterEach cleanup currently calls both
vi.restoreAllMocks() and vi.clearAllMocks() in the test file; remove the
redundant vi.clearAllMocks() call and keep only vi.restoreAllMocks() in the
afterEach block so mocks are restored and state cleared by restoreAllMocks()
(update the afterEach function that contains vi.restoreAllMocks() and
vi.clearAllMocks()).
cc3ea01 to
71cd8d3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@server/test/api/card.test.ts`:
- Around line 366-367: The inline comment above the expectation uses an
uppercase word "NOT" which violates the repo comment casing rule; edit the
comment preceding the expect(pax.addCapita).not.toHaveBeenCalled() assertion so
all words are lowercase (e.g., "pax should not be called for new users (no
upgrade)"), and apply the same lowercase fix to the other offending comment at
the mentioned location (lines around the second occurrence referencing
pax.addCapita).
- Around line 472-474: The test currently mocks persona.getAccount with a null
cast which violates its declared return type; update the vi.spyOn(...,
"getAccount").mockResolvedValueOnce(...) call to pass undefined (no casts) so it
matches Promise<AccountOutput<T> | undefined> and preserves type safety—i.e.,
replace the null as unknown as ... line with mockResolvedValueOnce(undefined).
501509c to
5cef43c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@server/test/api/card.test.ts`:
- Line 472: The test uses vi.spyOn(persona,
"getAccount").mockResolvedValueOnce() without an argument causing TS2554; update
the mock call to pass the expected return value explicitly (pass undefined) by
changing the mockResolvedValueOnce invocation for persona.getAccount to
mockResolvedValueOnce(undefined) so it matches the function signature
Promise<AccountOutput<T> | undefined>.
a8e392a to
353edeb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@server/test/mocks/pax.ts`:
- Around line 8-11: The mock for removeCapita is a Promise<void> but calls
mockResolvedValue() with no argument, which fails strict TypeScript typing;
update the removeCapita mock (the vi.fn<(internalId: string) => Promise<void>>()
mock) to call .mockResolvedValue(undefined) so the resolved value matches
Awaited<ReturnType<...>>; leave addCapita as-is.
353edeb to
a5b538a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@server/test/api/card.test.ts`:
- Around line 440-466: Update the "handles missing persona account during
signature card creation" test to explicitly mock persona.getAccount to return
undefined so the test doesn't depend on default mock behavior: inside that it
block add a spy/mock for getAccount (or the persona service method named
getAccount) and have it resolve to undefined before calling
appClient.index.$post, leaving existing expectations (e.g., vi.spyOn(pax,
"addCapita") and vi.spyOn(panda, "createCard")) unchanged.
04226a8 to
7643409
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
7643409 to
b65a3b2
Compare
c71e562 to
d4d2407
Compare
4375396 to
f1a488f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@server/api/card.ts`:
- Around line 346-348: The current boolean check creates isUpgradeFromPlatinum
by using credential.cards.filter(...).length > 0; replace this with
credential.cards.some(card => card.status === "DELETED" && card.productId ===
PLATINUM_PRODUCT_ID) to be more idiomatic and avoid creating an intermediate
array; update the assignment to use .some() on credential.cards and keep the
same variable name isUpgradeFromPlatinum.
- Around line 588-613: In handlePlatinumUpgrade, accessing
personaAccount.attributes.fields.documents.value can throw if documents is
undefined; change the extraction to safely read documents (e.g., use
attributes.fields.documents?.value ?? []), then keep the existing check for no
documents (if (!documents[0]) throw ...) and proceed to call addCapita; update
the reference in this function (handlePlatinumUpgrade,
personaAccount.attributes) so it uses the defensive pattern used elsewhere to
avoid a TypeError.
f1a488f to
827df2b
Compare
3987b77 to
1de797c
Compare
e5134a5 to
99e73b6
Compare
99e73b6 to
2a08bc7
Compare
Summary by CodeRabbit
New Features
Tests
Chores